EventStreams: Customisable Terminating Byte Sequence #115

paulhdk · 2024-09-17T01:36:06Z

Motivation

As discussed in apple/swift-openapi-generator#622, some APIs, e.g., ChatGPT or Claude, may return a non-JSON byte sequence to terminate a stream of events.
If not handled with a workaround (see below)such non-JSON terminating byte sequences cause a decoding error.

Modifications

This PR adds the ability to customise the terminating byte sequence by providing a closure to asDecodedServerSentEvents() as well as asDecodedServerSentEventsWithJSONData() that can match incoming data for the terminating byte sequence before it is decoded into JSON, for instance.

Result

Instead of having to decode and re-encode incoming events to filter out the terminating byte sequence - as seen in apple/swift-openapi-generator#622 (comment) - terminating byte sequences can now be cleanly caught by either providing a closure or providing the terminating byte sequence directly when calling asDecodedServerSentEvents() and asDecodedServerSentEventsWithJSONData().

Test Plan

This PR includes unit tests that test the new function parameters as part of the existing tests for asDecodedServerSentEvents() as well as asDecodedServerSentEventsWithJSONData().

simonjbeaumont

Just a courtesy note to acknowledge this PR—thanks for getting stuck in!

I’m out sick so might not get to this immediately. A quick glance: you’ll need to not remove the existing public initialiser (note that adding a parameter to a public method is technically API breaking because you can refer to functions by their signature.

czechboy0 · 2024-09-17T07:29:49Z

Thanks @paulhdk, the general shape of this is correct, let's just polish it up a bit together.

As Si mentioned, changing (even adding) function parameters is an API-breaking change, so we need to add the old variants to Deprecated.swift and mark it as deprecated (you'll see the many other deprecated methods in that name, just follow that pattern). That'll fix the first piece of feedback.
While the method you used to terminate the sequence should work, I think there's a simpler way to do this: AsyncSequence contains the prefix(while:) method, which is basically exactly what we need - we want the sequence to be iterated until a specific element is encountered, at which point we want to stop. That's equivalent to the prefix(while:) method, just with the test closure inverted (stop when the element matches a pattern). That means that you don't need to change the two implementation-only async sequences at all, you'll just need to take the input sequence and call the prefix(while:) method on it, and provide that. Let me try to add a suggestion on the diff to illustrate this better.
The naming of the parameter is interesting. terminate: is fine, but I think we can make it even clearer. terminator is used by print, but in that case, it adds the terminator to the rendered string, whereas here the parameter dictates when to stop iterating the elements. It might still work as a parameter name for the variant that provides ArraySlice<UInt8>, and terminateOn: for the (ArraySlice<UInt8>) -> Bool variant, but I'm happy for us to bikeshed that here a bit more, I hope we can make the name even better.

czechboy0

See comment above

paulhdk · 2024-09-17T18:31:55Z

Just a courtesy note to acknowledge this PR—thanks for getting stuck in!

Thank you both for all your help along the way!

I’m out sick so might not get to this immediately. A quick glance: you’ll need to not remove the existing public initialiser (note that adding a parameter to a public method is technically API breaking because you can refer to functions by their signature.

Thanks @paulhdk, the general shape of this is correct, let's just polish it up a bit together.

As Si mentioned, changing (even adding) function parameters is an API-breaking change, so we need to add the old variants to Deprecated.swift and mark it as deprecated (you'll see the many other deprecated methods in that name, just follow that pattern). That'll fix the first piece of feedback.

Ohh, I wasn't aware. But hadn't we discussed as an initial requirement that this PR shouldn't change the existing API? I thought about providing the changes of this PR in separate functions but I wasn't sure how to avoid duplicate code in that case. Is there an elegant way to preserve the existing API with this PR's changes?

While the method you used to terminate the sequence should work, I think there's a simpler way to do this: AsyncSequence contains the prefix(while:) method, which is basically exactly what we need - we want the sequence to be iterated until a specific element is encountered, at which point we want to stop. That's equivalent to the prefix(while:) method, just with the test closure inverted (stop when the element matches a pattern). That means that you don't need to change the two implementation-only async sequences at all, you'll just need to take the input sequence and call the prefix(while:) method on it, and provide that. Let me try to add a suggestion on the diff to illustrate this better.

Not sure if I'm missing something, but I'm not seeing your comment in the code? I just see "See comment above" but is not attached to any line of code.

The naming of the parameter is interesting. terminate: is fine, but I think we can make it even clearer. terminator is used by print, but in that case, it adds the terminator to the rendered string, whereas here the parameter dictates when to stop iterating the elements. It might still work as a parameter name for the variant that provides ArraySlice<UInt8>, and terminateOn: for the (ArraySlice<UInt8>) -> Bool variant, but I'm happy for us to bikeshed that here a bit more, I hope we can make the name even better.

Hm. What about processUntil: {}? In that case we would have to invert the return value of the closure though.
Or maybe terminatingByteReceived: {}?

czechboy0 · 2024-09-18T07:31:17Z

Ohh, I wasn't aware. But hadn't we discussed as an initial requirement that this PR shouldn't change the existing API? I thought about providing the changes of this PR in separate functions but I wasn't sure how to avoid duplicate code in that case. Is there an elegant way to preserve the existing API with this PR's changes?

The usual steps we take when replacing an API while preserving it in a deprecated form:

Move the existing function into Deprecated.swift
Mark it as deprecated
Copy it into the original location and make the modification (you've already done this)

We can usually manage to have the deprecated variant call into the new variant. In this case, I believe we should be able to make the deprecated variant call the new variant with a { false } closure for the terminator, meaning it'll never terminate prematurely and will always run through the full sequence.

Not sure if I'm missing something, but I'm not seeing your comment in the code? I just see "See comment above" but is not attached to any line of code.

Apologies, I got distracted and forgot to add the comment 🤦‍♂️ Let me do that now.

Hm. What about processUntil: {}? In that case we would have to invert the return value of the closure though.

Yeah I expect us to need to invert the value if we use prefix(while:) under the hood, but that's okay I think.

Since it's a closure with a boolean result, maybe it could be called isLastElement: (ArraySlice<UInt8>) -> Bool = { false }? Or shouldStop? I'll let @simonjbeaumont help us with the bikeshedding here 🙂

Sources/OpenAPIRuntime/EventStreams/ServerSentEventsDecoding.swift

paulhdk · 2024-09-30T17:33:22Z

@czechboy0 @simonjbeaumont just giving this a polite bump 🙂

I believe I’ve addressed all of your feedback so far. I just need clarification on one point above, @czechboy0.

Let me know if there’s anything missing.

czechboy0

Thanks for the ping, @paulhdk - I took a more detailed look and suggested a few more changes, mainly around the details of the sequence's state machine.

Sources/OpenAPIRuntime/EventStreams/ServerSentEventsDecoding.swift

Tests/OpenAPIRuntimeTests/EventStreams/Test_ServerSentEventsDecoding.swift

paulhdk · 2024-10-01T16:43:58Z

Thanks for the ping, @paulhdk - I took a more detailed look and suggested a few more changes, mainly around the details of the sequence's state machine.

Thanks for the review, @czechboy0! I’ve addressed everything and ran into some questions. See above.

czechboy0

Two more minor comments, otherwise lgtm

Sources/OpenAPIRuntime/EventStreams/ServerSentEventsDecoding.swift

simonjbeaumont · 2024-10-02T11:19:35Z

Just a courtesy note to all authors of open PRs. We have just cut over to GitHub Actions for CI so you will see different checks coming past. There is also a note in the CONTRIBUTING.md on how to run these locally: https://github.com/apple/swift-openapi-runtime/blob/main/CONTRIBUTING.md#run-ci-checks-locally.

a stream

Use `predicate` when referring to `while` internally, use `while` in the public API Invert the value of the return closure.

Default initialise with `{ _ in true }`

…EventsDecoding.swift Co-authored-by: Honza Dvorsky <[email protected]>

`init(upstream:)` as deprecated

…ventsDecoding.swift Co-authored-by: Honza Dvorsky <[email protected]>

…ams/ServerSentEventsDecoding.swift Co-authored-by: Honza Dvorsky <[email protected]>

…uence` initialiser

…ling `\n` Co-authored-by: Honza Dvorsky <[email protected]>

…it(upstream:)` as deprecated

`StateMachine`

…ding`

…ventsDecoding.swift Co-authored-by: Honza Dvorsky <[email protected]>

…izationSequence` iterator

czechboy0

Okay, added a few more nits, but this is now ready for CI - kicking off. Great work!

Sources/OpenAPIRuntime/EventStreams/ServerSentEventsDecoding.swift

…ventsDecoding.swift Co-authored-by: Honza Dvorsky <[email protected]>

Co-authored-by: Honza Dvorsky <[email protected]>

czechboy0

Looks great, thanks!

czechboy0 · 2024-10-03T16:34:53Z

You'll still need to rerun the formatter - see the updated CONTRIBUTING.md on how to use the act tool to run the format action locally, and commit the changes

paulhdk · 2024-10-03T17:21:45Z

You'll still need to rerun the formatter - see the updated CONTRIBUTING.md on how to use the act tool to run the format action locally, and commit the changes

PTAL. Ran the formatter locally. All checks should pass now!

czechboy0 · 2024-10-03T17:22:48Z

Thanks again for your contribution and for your patience as we've worked through all the details 👏 🙏

PSchmiedmayer · 2024-10-03T17:30:54Z

Congratulations @paulhdk; exciting to see this move into SpeziLLM and the work you have been doing there! 🚀

Thank you @czechboy0 & @simonjbeaumont for the support, reviews, and input!

paulhdk · 2024-10-03T17:34:00Z

Thanks again for your contribution and for your patience as we've worked through all the details 👏 🙏

Thank you @czechboy0 and @simonjbeaumont for your help! It was fun working this one out and I learnt lots!

czechboy0 · 2024-10-03T17:36:01Z

It was fun working this one out and I learnt lots!

Glad to hear it! If you're ever looking to contribute more, browse through https://github.com/apple/swift-openapi-generator/issues and comment on anything that catches your eye, and we can work together again 🙂

### Motivation See: apple/swift-openapi-runtime#115 ### Modifications This PR references the changes introduced by TODO in the documentation. ### Result Users planning to generate code for OpenAPI specs with custom terminating byte sequences will hopefully find it easier to make the necessary changes in their code. Please let me know if there are other places where these changes could be mentioned. ### Test Plan . --------- Co-authored-by: Honza Dvorsky <[email protected]>

paulhdk mentioned this pull request Sep 17, 2024

Docs: Reference Handling of Custom Terminating Byte Sequences apple/swift-openapi-generator#625

Merged

paulhdk changed the title ~~Customisable terminating byte sequence~~ EventStreams: Customisable Terminating Byte Sequence Sep 17, 2024

paulhdk mentioned this pull request Sep 17, 2024

Handling non-JSON payloads with asDecodedServerSentEventsWithJSONData() apple/swift-openapi-generator#622

Closed

simonjbeaumont reviewed Sep 17, 2024

View reviewed changes

czechboy0 requested changes Sep 17, 2024

View reviewed changes

czechboy0 reviewed Sep 18, 2024

View reviewed changes

Sources/OpenAPIRuntime/EventStreams/ServerSentEventsDecoding.swift Outdated Show resolved Hide resolved

Sources/OpenAPIRuntime/EventStreams/ServerSentEventsDecoding.swift Outdated Show resolved Hide resolved

czechboy0 reviewed Sep 18, 2024

View reviewed changes

Sources/OpenAPIRuntime/EventStreams/ServerSentEventsDecoding.swift Outdated Show resolved Hide resolved

simonjbeaumont added the semver/minor Adds new public API. label Sep 18, 2024

paulhdk force-pushed the customisable-terminating-byte-sequence branch 2 times, most recently from 9957e84 to 1487eda Compare September 20, 2024 18:02

czechboy0 requested changes Oct 1, 2024

View reviewed changes

paulhdk force-pushed the customisable-terminating-byte-sequence branch from a2b9089 to f14b5e8 Compare October 1, 2024 16:32

czechboy0 reviewed Oct 2, 2024

View reviewed changes

Sources/OpenAPIRuntime/EventStreams/ServerSentEventsDecoding.swift Outdated Show resolved Hide resolved

Sources/OpenAPIRuntime/EventStreams/ServerSentEventsDecoding.swift Outdated Show resolved Hide resolved

paulhdk and others added 11 commits October 2, 2024 21:35

EventStreams: add ability to customise the terminating byte sequence of

4325329

a stream

EventStreams: add tests for custom terminating byte sequences

0fa72a8

EventStreams: remove non-closure overload incl. its tests

5777ae0

EventStreams: rename "terminate" to "while"

b329be3

Use `predicate` when referring to `while` internally, use `while` in the public API Invert the value of the return closure.

EventStreams: make closure non-optional

625b013

Default initialise with `{ _ in true }`

Deprecated: mark asDecodedServerSentEvents as deprecated

a7d3791

Deprecated: mark asDecodedServerSentEventsWithJSONData as deprecated

514bbd0

Update doc comments in Sources/OpenAPIRuntime/EventStreams/ServerSent…

de8aeca

…EventsDecoding.swift Co-authored-by: Honza Dvorsky <[email protected]>

Deprecated: mark ServerSentEventsDeserializationSequence's

0057abe

`init(upstream:)` as deprecated

Update doc comment in Sources/OpenAPIRuntime/EventStreams/ServerSentE…

a346c61

…ventsDecoding.swift Co-authored-by: Honza Dvorsky <[email protected]>

Update doc comment in Sources/OpenAPIRuntime/EventStreams/ServerSentE…

54b1251

…ventsDecoding.swift Co-authored-by: Honza Dvorsky <[email protected]>

paulhdk and others added 10 commits October 2, 2024 21:35

Update doc comment in Sources/OpenAPIRuntime/EventStreams/ServerSentE…

feaba23

…ventsDecoding.swift Co-authored-by: Honza Dvorsky <[email protected]>

Remove redundant forced type cast in Sources/OpenAPIRuntime/EventStre…

d286aec

…ams/ServerSentEventsDecoding.swift Co-authored-by: Honza Dvorsky <[email protected]>

EventStreams: formatting

36df44b

EventStreams: add doc comment for `ServerSentEventsDeserializationSeq…

6de6ec2

…uence` initialiser

EventStreams: check for terminating byte sequence after removing trai…

4e7bdb5

…ling `\n` Co-authored-by: Honza Dvorsky <[email protected]>

fixup! Deprecated: mark ServerSentEventsDeserializationSequence's `in…

ad8f055

…it(upstream:)` as deprecated

EventStreams: store predicate closure as an associated value in

1552be8

`StateMachine`

EventStreams: remove eventCountOffset in `Test_ServerSentEventsDeco…

2c1b3cb

…ding`

Update doc comment in Sources/OpenAPIRuntime/EventStreams/ServerSentE…

97bda70

…ventsDecoding.swift Co-authored-by: Honza Dvorsky <[email protected]>

EventStream: remove predicate property in `ServerSentEventsDeserial…

0d4345e

…izationSequence` iterator

paulhdk force-pushed the customisable-terminating-byte-sequence branch from a770b5f to 0d4345e Compare October 3, 2024 04:35

czechboy0 reviewed Oct 3, 2024

View reviewed changes

Sources/OpenAPIRuntime/EventStreams/ServerSentEventsDecoding.swift Outdated Show resolved Hide resolved

Sources/OpenAPIRuntime/EventStreams/ServerSentEventsDecoding.swift Outdated Show resolved Hide resolved

paulhdk and others added 4 commits October 3, 2024 07:20

Update doc comment in Sources/OpenAPIRuntime/EventStreams/ServerSentE…

0b841db

…ventsDecoding.swift Co-authored-by: Honza Dvorsky <[email protected]>

EventStreams: address CI warnings re: doc comments

30c9b81

EventStream: only assign state once in each pass in next()

31462e6

Co-authored-by: Honza Dvorsky <[email protected]>

Merge branch 'main' into customisable-terminating-byte-sequence

86d0966

czechboy0 approved these changes Oct 3, 2024

View reviewed changes

Format

78e19da

czechboy0 merged commit d604dd0 into apple:main Oct 3, 2024
18 checks passed

paulhdk deleted the customisable-terminating-byte-sequence branch October 3, 2024 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EventStreams: Customisable Terminating Byte Sequence #115

EventStreams: Customisable Terminating Byte Sequence #115

paulhdk commented Sep 17, 2024

simonjbeaumont left a comment

czechboy0 commented Sep 17, 2024

czechboy0 left a comment

paulhdk commented Sep 17, 2024 •

edited

Loading

czechboy0 commented Sep 18, 2024

paulhdk commented Sep 30, 2024

czechboy0 left a comment

paulhdk commented Oct 1, 2024

czechboy0 left a comment

simonjbeaumont commented Oct 2, 2024

czechboy0 left a comment

czechboy0 left a comment

czechboy0 commented Oct 3, 2024 •

edited

Loading

paulhdk commented Oct 3, 2024 •

edited

Loading

czechboy0 commented Oct 3, 2024

PSchmiedmayer commented Oct 3, 2024

paulhdk commented Oct 3, 2024

czechboy0 commented Oct 3, 2024

EventStreams: Customisable Terminating Byte Sequence #115

EventStreams: Customisable Terminating Byte Sequence #115

Conversation

paulhdk commented Sep 17, 2024

Motivation

Modifications

Result

Test Plan

simonjbeaumont left a comment

Choose a reason for hiding this comment

czechboy0 commented Sep 17, 2024

czechboy0 left a comment

Choose a reason for hiding this comment

paulhdk commented Sep 17, 2024 • edited Loading

czechboy0 commented Sep 18, 2024

paulhdk commented Sep 30, 2024

czechboy0 left a comment

Choose a reason for hiding this comment

paulhdk commented Oct 1, 2024

czechboy0 left a comment

Choose a reason for hiding this comment

simonjbeaumont commented Oct 2, 2024

czechboy0 left a comment

Choose a reason for hiding this comment

czechboy0 left a comment

Choose a reason for hiding this comment

czechboy0 commented Oct 3, 2024 • edited Loading

paulhdk commented Oct 3, 2024 • edited Loading

czechboy0 commented Oct 3, 2024

PSchmiedmayer commented Oct 3, 2024

paulhdk commented Oct 3, 2024

czechboy0 commented Oct 3, 2024

paulhdk commented Sep 17, 2024 •

edited

Loading

czechboy0 commented Oct 3, 2024 •

edited

Loading

paulhdk commented Oct 3, 2024 •

edited

Loading